Skip to content

OCTRL-1078 Make Task Controller able to control OCC tasks#804

Open
justonedev1 wants to merge 1 commit intomasterfrom
OCTRL-1078
Open

OCTRL-1078 Make Task Controller able to control OCC tasks#804
justonedev1 wants to merge 1 commit intomasterfrom
OCTRL-1078

Conversation

@justonedev1
Copy link
Collaborator

introduction of controller for kubernetes inside the controller-operator folder.

This version allows us to control all OCC tasks (tested on readout, stfsender and stfbuilder). Currently there is not image and controller was run directly via make run on the node where the kubernetes cluster is running. If you run it locally and cluster is on different computer, you would not be able to communicate with OCC process as these require opened ports
Manifests required to run these tasks are inside the ecs-manifests subfolder.

In order to apply these manifests use kubectl. There is an explanation which file is which inside the kubernetes-ecs.md.

Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I did not try to run it yet, but I already have some questions/suggestions.

* === This file is part of ALICE O² ===
*
* Copyright 2024 CERN and copyright holders of ALICE O².
* Author: Teo Mrnjavac <teo.mrnjavac@cern.ch>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Author: Teo Mrnjavac <teo.mrnjavac@cern.ch>

applies to the rest as well.

Comment on lines +1 to +15
/*
Copyright 2024.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a thought, we might want to know whether we should keep the original licences of auto-generated files or move to the one we use in O2.

kind: Kustomization
images:
- name: controller
newName: docker.io/teom/aliecs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's valid? see also the Makefile which refers to teom

- O2_PARTITION={{ environment_id }}
user: "{{ user }}"
arguments:
- "/root/readout.yaml"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the manifest file added as an argument to readout?

user: "{{ user }}"
value: "{{ len(modulepath)>0 ? _module_cmdline : _plain_cmdline }}"
arguments:
- "/root/stfbuilder-senderoutput.yaml"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here

)

// fmqToOCCState maps FairMQ internal states to lowercase OCC state names
var fmqToOCCState = map[string]string{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we do it a bit differently in the current system: https://github.com/AliceO2Group/Control/blob/master/core/integration/odc/fairmq/fairmq.go

It probably doesn't matter much in here though.

@@ -0,0 +1,69 @@
apiVersion: aliecs.alice.cern/v1alpha1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this and the other manifest in this directory could be moved to ecs-manifests.

@@ -0,0 +1,306 @@
# VERSION defines the project version for the bundle.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my curiousity, this file is auto-generated by the operator SDK or AI-generated?

log.V(1).Info("new reconcile request on existing Task Kind", "request", req)

// Handle finalizers for clean deletion
if res, stop, err := r.handleFinalizer(ctx, t, log); err != nil || stop {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am struggling to understand what it is needed for, could you explain please?

Comment on lines +148 to +149
// As far as I understand this is necessary because even though we have gRPC streams, we cannot rely
// on them being implemented
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean that they might not be implemented? Why do we recognize it by seeing an empty t.Status.State?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants